-
Notifications
You must be signed in to change notification settings - Fork 239
Add Nvfatbin Bindings #1467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add Nvfatbin Bindings #1467
Conversation
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
FYI @rparolin all of these are generated code except for the tests. There is an internal MR targeting the codegen for this. |
cuda_bindings/tests/test_nvfatbin.py
Outdated
| nvcc = shutil.which("nvcc") | ||
| if nvcc is None: | ||
| pytest.skip("nvcc not found on PATH") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a #TODO comment to use pathfinder to resolve this in the future instead? I imagine this wouldn't cover using the nvidia-cuda-nvcc wheel package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a TODO is fine, though it's unlikely actionable in the near future. We do not have access to host compilers in the CI (at the test stage) so NVCC is useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively I can also embed an object file in this test script, just like the tileIR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way is fine as long as we know how to reproduce the embedded bytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just looked up the size of an object file - with the small kernel here it's over 1.2M. Unlike tile IR this will clutter the test file. I'm going to leave this as a TODO for now.
| if err != nvrtc.nvrtcResult.NVRTC_SUCCESS: | ||
| raise RuntimeError(repr(err)) | ||
|
|
||
| err, program_handle = nvrtc.nvrtcCreateProgram(CODE.encode(), b"", 0, [], []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker for this PR, but I think we have some helper functions for NVRTC that can be used in cuda-bindings tests, so that we do not have to reimplement them in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I saw this pattern showed up in test_nvjitlink as well. Perhaps a shared fixture that returns an nvrtc artifact.
|
/ok to test 8e75d8a |
|
I suspect it's coming from
|
|
|
/ok to test 965c53b |
Need to skip the tile IR test for nvFatbin < 13.1 |
This shows in some CI environment there's nvcc installed but it's not properly setup due to missing host compiler. |
|
/ok to test 4baeecd |
|
/ok to test 4c2f3f3 |
Yup! See #1467 (comment). |
| [project.optional-dependencies] | ||
| all = [ | ||
| "cuda-toolkit[nvrtc,nvjitlink,nvvm]==13.*", | ||
| "cuda-toolkit[nvrtc,nvjitlink,nvvm,nvfatbin]==13.*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kkraus14 this minds me a discussion we had a few months ago -- One day installing cuda-binding[all] would bring in the whole CTK 😂
|
LGTM! Thanks, @isVoid! |
Description
closes #156
Checklist